Skip to content

Conversation

@janhesters
Copy link
Collaborator

Summary

  • Add riteway/bun adapter that provides the familiar assert API with Bun's fast, built-in test runner
  • Include TypeScript type definitions (bun.d.ts)
  • Add comprehensive tests for the Bun adapter
  • Update README with Bun usage documentation and examples

Test plan

  • Run bun test source/bun.test.js to verify the adapter works correctly
  • Verify TypeScript types work with import { test, describe, assert } from 'riteway/bun'
  • Test failure output includes given and should context

🤖 Generated with Claude Code

@janhesters janhesters self-assigned this Jan 27, 2026
@janhesters janhesters force-pushed the feat/bun-test-support branch 2 times, most recently from 4f8226a to 10d79f9 Compare January 27, 2026 09:59
@ericelliott
Copy link
Collaborator

@cursor please /review - and point out any lost functionality or regressions this may cause, if any. Don't hallucinate.

@cursor
Copy link

cursor bot commented Feb 2, 2026

Code Review: Bun Test Runner Support

I've completed a thorough review of the PR. Here are my findings:

✅ Implementation Quality

The Bun adapter follows the correct pattern established by the Vitest adapter:

  1. assert() function - Correctly implements the required given, should, actual, expected parameters with proper validation
  2. Custom matcher - Uses toRitewayEqual to integrate with Bun's expect API
  3. Error messages - Properly formats failure output with "Given X: should Y" context as specified in requirements
  4. TypeScript types - Includes proper type definitions in bun.d.ts
  5. Re-exports - Correctly re-exports test and describe from bun:test

📋 Observed Differences (Not Regressions)

These are intentional adapter design patterns:

  1. Missing utilities (Try, createStream, countKeys): The Bun adapter doesn't export these, but this matches the Vitest adapter pattern. Looking at source/vitest.test.jsx, I can see tests import these utilities directly from the core module:
import { Try, countKeys, createStream } from './riteway';

Users can do the same with Bun - import utilities from riteway and assert from riteway/bun.

  1. Setup requirement (setupRitewayBun()): This is necessary due to Bun's expect.extend() API. The README properly documents using Bun's preload configuration to handle this cleanly.

  2. No .only and .skip variants: The adapter re-exports Bun's native test and describe, which have their own .only and .skip support built-in. Users can access these via: test.only(), test.skip(), describe.only(), describe.skip().

🔍 Missing Test Coverage

The Bun test file (source/bun.test.js) is more limited than other test files. It should include tests for:

  1. Error validation - Testing that missing required keys throw proper errors (similar to lines 79-111 in source/test.js)
  2. Failure cases - Testing what happens when assertions fail (currently only tests passing assertions)
  3. Edge cases - Testing with undefined values, nested structures with differences, etc.

📝 Documentation Quality

The README updates are comprehensive and follow the same pattern as the Vitest section. Good examples and clear setup instructions.

⚠️ Potential Issues

One actual regression concern:

Looking at lines 33-42 in source/bun.js, the assert() function validates required keys before calling the matcher. However, unlike the core riteway (line 16 in source/riteway.js), it doesn't use .reduce(concatToString, '') for formatting the error message. Instead it uses .join(', ').

This means error messages will differ slightly:

  • Core riteway: "The following parameters are required by \assert`: given, should, actual, expected"`
  • Bun adapter: "The following parameters are required by \assert`: given, should, actual, expected"` ✓

Actually, checking the implementation more carefully, both produce the same result - .join(', ') is cleaner than the reduce pattern. Not a regression.

✅ Verdict: No Regressions

This PR adds Bun support without causing regressions because:

  • It follows the adapter pattern established by Vitest
  • Core riteway functionality remains unchanged
  • Utility functions remain available from the main module
  • All required assert functionality is present
  • Error messages maintain the required format
  • TypeScript types are provided

Recommendation: Approve with suggestion to enhance test coverage for error cases.

Open in Cursor Open in Web

ericelliott
ericelliott previously approved these changes Feb 2, 2026
Copy link
Collaborator

@ericelliott ericelliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@ericelliott
Copy link
Collaborator

  • remove travis.yml
  • add github action to run the tests

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for Bun's built-in test runner to the riteway testing library. The implementation provides a familiar assert API that works with Bun's fast, built-in test runner through a custom matcher extension.

Changes:

  • Added source/bun.js adapter with assert function and setupRitewayBun() for registering custom matchers
  • Added TypeScript definitions in bun.d.ts
  • Created test suite in source/bun.test.js validating the adapter functionality
  • Updated README with comprehensive Bun usage documentation including setup, examples, and failure output
  • Added package.json export configuration for riteway/bun

Reviewed changes

Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
source/bun.js Core Bun adapter implementation with assert function and custom matcher setup
source/bun.test.js Test suite for the Bun adapter covering basic assertion scenarios
bun.d.ts TypeScript type definitions for the Bun adapter exports
package.json Added export path for riteway/bun module
README.md Added comprehensive documentation section for Bun integration with setup instructions and examples
.claude/settings.local.json Claude Code tool configuration (development artifact)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1 to +50
import { test, describe, assert, setupRitewayBun } from './bun.js';

setupRitewayBun();

describe('riteway/bun', () => {
test('given: matching primitives, should: pass', () => {
assert({
given: 'two identical numbers',
should: 'be equal',
actual: 42,
expected: 42,
});
});

test('given: matching strings, should: pass', () => {
assert({
given: 'two identical strings',
should: 'be equal',
actual: 'hello',
expected: 'hello',
});
});

test('given: matching objects, should: pass', () => {
assert({
given: 'two identical objects',
should: 'be deeply equal',
actual: { name: 'Bun', version: 1.1 },
expected: { name: 'Bun', version: 1.1 },
});
});

test('given: matching arrays, should: pass', () => {
assert({
given: 'two identical arrays',
should: 'be deeply equal',
actual: [1, 2, 3],
expected: [1, 2, 3],
});
});

test('given: nested structures, should: pass', () => {
assert({
given: 'two identical nested structures',
should: 'be deeply equal',
actual: { users: [{ name: 'Alice' }, { name: 'Bob' }] },
expected: { users: [{ name: 'Alice' }, { name: 'Bob' }] },
});
});
});
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test file only covers happy path scenarios where all assertions pass. Based on the vitest test suite (source/vitest.test.jsx lines 72-107), the bun adapter tests should also include:

  1. Tests for assert() error handling when required parameters are missing (e.g., calling assert({}) or assert({ given: 'x', should: 'y' }) without actual/expected)
  2. Tests to verify the error message format matches: "The following parameters are required by assert: [missing keys]"
  3. Tests for undefined values to ensure they don't throw errors when all parameters are provided

These tests would verify that the parameter validation logic in source/bun.js works correctly.

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +21
"./bun": {
"import": "./source/bun.js",
"types": "./bun.d.ts"
},
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the vitest adapter (which has exports at lines 54-66 including "./esm/vitest.js" and "./esm/vitest.test.jsx"), consider adding corresponding esm exports for the bun adapter. This would include:

  • "./esm/bun.js" pointing to "./source/bun.js" with types "./bun.d.ts"
  • "./esm/bun.test.js" pointing to "./source/bun.test.js"

These exports appear to support the package's esm build process (see the "esm" script at line 77).

Copilot uses AI. Check for mistakes.
@janhesters janhesters force-pushed the feat/bun-test-support branch 5 times, most recently from 9b4e8a2 to 69d5dab Compare February 3, 2026 11:28
Add riteway/bun adapter for Bun's built-in test runner, following the
same pattern as the existing Vitest integration.

- Add source/bun.js with assert function, setupRitewayBun, and re-exported test/describe
- Add bun.d.ts TypeScript definitions
- Add source/bun.test.js with tests
- Update package.json exports
- Update README.md with Bun documentation
- Replace Travis CI with GitHub Actions

Usage:
  import { test, describe, assert, setupRitewayBun } from 'riteway/bun';

  setupRitewayBun();

  test('example', () => {
    assert({ given: 'x', should: 'y', actual: 1, expected: 1 });
  });

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@janhesters janhesters force-pushed the feat/bun-test-support branch from 69d5dab to 08e0061 Compare February 3, 2026 11:29
@ianwhitedeveloper
Copy link
Collaborator

Code Review: PR #396 - feat/bun-test-support

Validation:npm test (37/37 passing), npm run lint, npm run ts — all passing.

Blocking Issue: Test coverage for the Bun adapter (source/bun.test.js) only covers happy-path assertions. Missing tests for error cases (missing required keys, error message format, undefined values, failure scenarios) — does not match the thoroughness of the existing Vitest test suite (source/vitest.test.jsx:72-107).

Verdict: ⚠️ Request Changes — Address test coverage gap before merge.


Full Review Details

Scope Analysis

The PR bundles three distinct concerns:

Concern Files In Scope? Notes
Bun adapter source/bun.js, source/bun.test.js, bun.d.ts, package.json (exports), README.md, vitest.config.js ✅ Yes Core feature, properly implemented
CI migration .github/workflows/ci.yml, .travis.yml (deleted) ⚠️ Acceptable Explicitly requested in PR comments; aligns with Context7 GitHub Action epic in tasks/
bin rename + TS config bin/ritewaybin/riteway.js, bin/riteway.test.js, package.json (typecheck with --skipLibCheck) ⚠️ Separate concern ESM requirement fix + type suppression; would benefit from its own commit

Conclusion: While the PR technically introduces scope creep by bundling CI migration and bin fixes, these changes are pragmatic given:

  1. CI migration was explicitly requested and aligns with planned infrastructure work
  2. The bin extension fix resolves ESM import issues
  3. All changes are related to test infrastructure modernization

Test Coverage Gap (Blocking)

Current Coverage Analysis:

source/bun.test.js (lines 5-50) only tests passing assertions across data types:

  • ✅ Primitives (numbers, strings)
  • ✅ Objects (shallow and nested)
  • ✅ Arrays

Missing Critical Test Cases:

Comparing to source/vitest.test.jsx:72-107 and source/test.js:79-111:

Test Scenario Core/Vitest Bun Impact
Missing all required keys Can't verify error handling works
Missing partial keys Can't verify which keys are validated
Undefined values don't throw Can't verify edge case handling
Error message format validation Can't verify helpful error messages
Failure assertion scenarios Can't verify matcher output format

Required Test Additions:

// 1. Missing required keys validation
test('given: missing required keys, should: throw with helpful message', () => {
  let error;
  try {
    assert({ given: 'test' }); // missing should, actual, expected
  } catch (e) {
    error = e;
  }
  
  assert({
    given: 'incomplete assertion object',
    should: 'throw error',
    actual: error instanceof Error,
    expected: true
  });
  
  assert({
    given: 'incomplete assertion object',
    should: 'list missing keys in error message',
    actual: error.message.includes('should, actual, expected'),
    expected: true
  });
});

// 2. Undefined values should not throw
test('given: undefined values, should: not throw validation error', () => {
  let error;
  try {
    assert({
      given: 'test',
      should: 'handle undefined',
      actual: undefined,
      expected: undefined
    });
  } catch (e) {
    error = e;
  }
  
  assert({
    given: 'undefined in actual and expected',
    should: 'not throw validation error',
    actual: error === undefined,
    expected: true
  });
});

// 3. Failure case with proper context
test('given: failing assertion, should: include given/should context', () => {
  let error;
  try {
    assert({
      given: 'two different numbers',
      should: 'be equal',
      actual: 42,
      expected: 43
    });
  } catch (e) {
    error = e;
  }
  
  assert({
    given: 'failing assertion',
    should: 'include given context',
    actual: error.message.includes('two different numbers'),
    expected: true
  });
  
  assert({
    given: 'failing assertion',
    should: 'include should context',
    actual: error.message.includes('be equal'),
    expected: true
  });
});

Implementation Quality Analysis

Strengths:

  1. Functional programming principles — Follows javascript.mdc guidelines:

    • Uses arrow functions, destructuring, parameter defaults
    • Pure functions with clear single responsibilities
    • No mutation, proper const usage
  2. Pattern consistency — Matches Vitest adapter architecture:

    • assert() validates required keys before delegation
    • Re-exports test and describe from native runner
    • Utility functions (Try, countKeys) importable from core
  3. Custom matcher implementation — Correctly follows Bun's API:

    • Uses this.equals() for deep equality comparison
    • Uses this.utils.printExpected/printReceived for formatting
    • Returns { pass: boolean, message: () => string } as required
  4. TypeScript definitions — Clean, type-safe interface:

    • Generic <T> allows type inference
    • readonly properties enforce immutability
    • Proper module declaration with re-exports
  5. Documentation — Comprehensive README section:

    • Clear installation and setup instructions
    • Documents preload configuration pattern
    • Includes practical examples and failure output

Code Quality Details:

// source/bun.js:33-42 - assert implementation
export const assert = (args = {}) => {
  const missing = requiredKeys.filter((k) => !Object.keys(args).includes(k));
  if (missing.length) {
    throw new Error(
      `The following parameters are required by \`assert\`: ${missing.join(', ')}`
    );
  }

  const { given, should, actual, expected } = args;
  expect(actual).toRitewayEqual(expected, given, should);
};

Observation: Uses .join(', ') while core uses .reduce(concatToString, ''). Both produce identical output, but .join() is cleaner (KISS principle). Consider standardizing core to match.

Non-Blocking Issues

Priority 1: Setup Fragility

setupRitewayBun() must be called before tests run. If users misconfigure preload or forget to call it, they get:

TypeError: toRitewayEqual is not a function

Improvement options:

  1. Add lazy initialization check in assert()
  2. Provide clearer error message if matcher not registered
  3. Document common setup errors in README

Priority 2: TypeScript Configuration Issues

// package.json:74 - overly broad suppression
"typecheck": "... --skipLibCheck ..."

Problems:

  • Suppresses ALL library type errors, not just bun:test
  • Could mask regressions in other dependencies
  • @ts-nocheck in bin/riteway.test.js is blanket suppression

Better approach:

// Target specific file exclusion
"typecheck": "... && npx -p typescript tsc bun.d.ts --noEmit --skipLibCheck ..."

Priority 3: TypeScript Enhancement

Per Bun best practices, add declaration merging for better IDE support:

// Add to bun.d.ts
declare module 'bun:test' {
  interface Matchers<T> {
    toRitewayEqual(expected: T, given: string, should: string): void;
  }
  interface AsymmetricMatchers {
    toRitewayEqual(expected: unknown, given: string, should: string): void;
  }
}

Priority 4: CI Workflow Improvements

# .github/workflows/ci.yml:45-52
- name: Setup Bun
  uses: oven-sh/setup-bun@v2  # Not pinned - uses latest

Recommendations:

  1. Pin Bun version for reproducibility: with: bun-version: 1.1.29
  2. Add Node v22 LTS to test matrix (currently only tests v20)
  3. Add Bun dependency caching: cache: 'bun' (like npm cache)

Priority 5: Missing ESM Export Path

Vitest has ./esm/vitest.js export mapping, Bun adapter doesn't:

// Missing from package.json exports
"./esm/bun.js": {
  "import": "./source/bun.js",
  "types": "./bun.d.ts"
}

Consider adding for consistency with existing adapter patterns.

Priority 6: Documentation Gap

README doesn't mention utility functions aren't re-exported. Add:

### Utility Functions

The Bun adapter focuses on the core `assert` API. For utility functions like `Try`, `countKeys`, or `createStream`, import them from the main riteway module:

```typescript
import { Try, countKeys } from 'riteway';
import { test, describe, assert } from 'riteway/bun';

### CI/CD Configuration Review

**GitHub Actions Workflow Analysis:**

```yaml
jobs:
  test:  # Node.js test job
    - npm install --legacy-peer-deps
    - npm run lint
    - npm run typecheck
    - npm test
  
  bun-test:  # Separate Bun job
    - bun install
    - bun test source/bun.test.js

Strengths:

  • ✅ Separate jobs provide isolation
  • ✅ Uses official oven-sh/setup-bun@v2 action
  • ✅ Uses latest GitHub Actions versions (v4)
  • workflow_dispatch allows manual runs
  • ✅ Maintains existing Node test pipeline

Infrastructure Context:
Aligns with Context7 GitHub Action epic (tasks/2025-01-27-context7-github-action-epic.md) which planned GitHub Actions infrastructure. This PR lays necessary groundwork.

Security & Performance

Security:

  • ✅ No new dependencies added
  • ✅ Uses Bun's built-in APIs (no third-party matcher libraries)
  • ✅ Input validation prevents partial assertions
  • ✅ No security-sensitive file changes

Performance:

  • ✅ Bun is significantly faster than Node for test execution
  • ✅ Parallel job execution in CI
  • ✅ No performance regressions to existing test runners
  • ✅ All 37 tests pass in < 2 seconds locally

Backward Compatibility & Regressions

Analysis:

Component Status Evidence
Core riteway tests ✅ No regression 31/31 tests pass
Vitest adapter ✅ Unaffected 6/6 tests pass
CLI functionality ✅ Unchanged bin tests pass
All exports ✅ Maintained Type checks pass
Travis CI ⚠️ Removed Replaced by GitHub Actions

Verdict: No functional regressions. Travis → GitHub Actions is an infrastructure upgrade, not a regression.

Adapter Pattern Consistency

Comparing implementation across all test runners:

Feature Core (tape) Vitest Bun Assessment
assert() with validation ✅ Consistent
describe ✅ Native ⚠️ Re-export ⚠️ Re-export ✅ Appropriate delegation
test N/A ⚠️ Re-export ⚠️ Re-export ✅ Appropriate delegation
.only() / .skip() ✅ Native ✅ Native ✅ Available via re-exports
Try utility ✅ Import from core
countKeys utility ✅ Import from core
createStream ✅ Import from core
Setup required ⚠️ Yes ⚠️ Bun API limitation

Conclusion: Bun adapter correctly follows established Vitest adapter pattern. Setup requirement is unavoidable due to Bun's expect.extend() API design.

TDD Compliance (per tdd.mdc)

5 Questions Framework:

Checking source/bun.test.js:5-13:

describe('riteway/bun', () => {
  test('given: matching primitives, should: pass', () => {
    assert({
      given: 'two identical numbers',  // ✅ Question 2: Expected behavior
      should: 'be equal',              // ✅ Question 2: Expected behavior
      actual: 42,                       // ✅ Question 3: Actual output
      expected: 42,                     // ✅ Question 4: Expected output
    });
  });
  1. Unit under test?riteway/bun (describe block)
  2. Expected behavior? → Clear given/should statements
  3. Actual output? → Explicitly tested
  4. Expected output? → Explicitly declared
  5. How to reproduce? → Complete test context provided

Gap: Only tests passing cases. Missing failure scenarios means Question 5 isn't fully answered for error conditions.

Official Documentation Alignment

Bun Test Runner Best Practices (verified via web search):

From Bun documentation:

  1. Custom matcher signature correct:

    expect.extend({
      matcherName(received, ...args) {
        return { pass: boolean, message: () => string };
      }
    });
  2. Uses this.equals() for deep equality (recommended over manual comparison)

  3. Uses this.utils for formatting (printExpected/printReceived)

  4. ⚠️ Missing TypeScript declaration merging (recommended for full type support)


Summary & Recommendations

❌ Blocking (Must Fix Before Merge)

  1. Add comprehensive error case tests to source/bun.test.js:
    • Missing required keys validation
    • Partial missing keys error messages
    • Undefined values handling
    • Failure scenario output format

⚠️ High Priority (Recommended)

  1. Improve TypeScript configuration:

    • Replace blanket --skipLibCheck with targeted exclusion
    • Remove @ts-nocheck from bin/riteway.test.js, use targeted suppressions
    • Add Bun matcher type declaration merging
  2. Enhance setup robustness:

    • Add guard message if setupRitewayBun() not called
    • Document common setup errors in README

📝 Medium Priority (Nice to Have)

  1. CI workflow improvements:

    • Pin Bun version for reproducibility
    • Add Node v22 to test matrix
    • Add Bun dependency caching
  2. Documentation enhancement:

    • Add note about importing utility functions from core
    • Document .only() and .skip() availability
  3. Package consistency:

    • Add ./esm/bun.js export mapping

Final Verdict

⚠️ REQUEST CHANGES — The implementation is architecturally sound and follows best practices, but the test coverage gap is a blocking issue. The Bun adapter needs the same level of test thoroughness as the Vitest adapter to ensure reliability and maintainability.

Once test coverage is addressed, this PR will be ready to merge.

Positive Notes:

  • Clean, functional code following project standards
  • Proper documentation and TypeScript definitions
  • Good CI infrastructure modernization
  • No regressions to existing functionality
  • Aligns with planned infrastructure work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants